Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update trec_eval Fix #475 #477

Merged
merged 4 commits into from Nov 16, 2018
Merged

Update trec_eval Fix #475 #477

merged 4 commits into from Nov 16, 2018

Conversation

chriskamphuis
Copy link
Collaborator

Updated trec_eval to latest version. Running old version can result in a
segmentation fault.

Updated trec_eval to latest version. Running old version can result in a
segmentation fault.
@lintool
Copy link
Member

lintool commented Nov 16, 2018

Hi @chriskamphuis I only see removal of the existing tarball? There isn't a new one?

@chriskamphuis
Copy link
Collaborator Author

Hi @lintool, I replaced it with a tarball with the same name. The commit shows that the file is modified (not deleted).

@lintool
Copy link
Member

lintool commented Nov 16, 2018

Ah, I see - misread.

The new tarball is created from https://github.com/usnistgov/trec_eval/ ?
I think it would reduce confusion to change the name? I think the latest version is 9.0.4? Might want to include the GitHub commit id in this PR to be absolutely clear.

Tarball is created from usnistgov/trec_eval with
a5211566d0c9e2ec337bacf327b9350ab5b3edde as latest commit.
@chriskamphuis
Copy link
Collaborator Author

@lintool

I included the commit id in the commit that changed the name, the commit id of usnistgov/trec_eval is a5211566d0c9e2ec337bacf327b9350ab5b3edde.

@lintool
Copy link
Member

lintool commented Nov 16, 2018

@chriskamphuis sorry to be kinda a pain, but we're try to implement rigorous software engineering practices in this project - CI is failing because of this -

https://github.com/castorini/Anserini/blob/master/.travis.yml#L16

File name needs to be updated. Can you please do so?

@chriskamphuis
Copy link
Collaborator Author

@lintool Yes, no problem, I am working on it. I will also update the markdown files so it will be consistent.

Renamed in all relevant files
@@ -60,47 +60,47 @@ nohup target/appassembler/bin/SearchCollection -topicreader Webxml -index lucene
Evaluation can be performed using `trec_eval` and `gdeval.pl`:

```
eval/trec_eval.9.0/trec_eval -m map -m P.30 src/main/resources/topics-and-qrels/qrels.web.51-100.txt run.cw09b.bm25.topics.web.51-100.txt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit odd why there is an extra empty line here.
Did you manually change this file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Peilin-Yang are those parts auto-gen'ed? Perhaps @chriskamphuis doesn't know about the details of the regression framework...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All experiments-{$collection}.md files are auto-generated.
@chriskamphuis Could you please run mvn test locally and see if there is any diff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Peilin-Yang No diff for me locally. I only changed the the index.md and yaml files. Then I ran test before pushing the changes. Maybe I accidentally added an extra line when checking if everything was correct, but then it should be removed when running mvn test right?

@lintool lintool merged commit b4064da into castorini:master Nov 16, 2018
@lintool
Copy link
Member

lintool commented Nov 16, 2018

@chriskamphuis thanks for your first contribution. Looking forward to many more in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants